Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport of #735 to secp256k1-sys 0.10 #736

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

apoelstra
Copy link
Member

This backports #735. I am PR'ing to the secp256k1-0.29.x branch because this will work and because it feels like unnecessary complication to try to create a secp256k1-sys-0.10.x branch, which might be "more correct" but would make our git tree harder to understand and maintain.

As in #735, this just deletes some dead code from secp256k1-sys/depend/secp256k1.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 10, 2024

Do we want to ignore CI?

@apoelstra
Copy link
Member Author

Yes. We only just started making an effort to fix CI to pin tooling last week. We don't want to try to backport all those changes.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bb026c4

@apoelstra
Copy link
Member Author

Forgot to update the lockfiles (which in 0.29.x exist but are not checked in CI). Will get my local CI working on this before merging..

Copy link
Member Author

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successfully ran local tests on 21f67d5.

@apoelstra
Copy link
Member Author

Note to self: needed to add

SECP_VENDOR_VERSION_CODE=0_10_0 \

to my local CI script to get it to pass, since by default it wants the symbols in secp-sys to match the version in Cargo.toml exactly. (But doing this would turn a non-breaking change into a breaking one, which is why I didn't.)

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 11, 2024

@apoelstra your CI fail made me realize we should do the same thing here as we do in bitcoinconsensus.

@apoelstra
Copy link
Member Author

What in particular?

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 11, 2024

Version the API, not the library and put the upstream version into metadata instead.

@apoelstra
Copy link
Member Author

Agreed. Though for purposes of this PR, we can't make such a change (as it'd be a breaking change to change our version format at all).

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 11, 2024

Well, we already are breaking our format here, but obviously I'm not NACKing this.

@apoelstra
Copy link
Member Author

obviously I'm not NACKing this

Will you ACK it? :}

Fine if you don't want to, but there aren't a lot of maintainers on this repo and you're the one who appears to be active right now.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 11, 2024

I already did and since GH didn't dismiss my review I haven't noticed the force push.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 21f67d5

@apoelstra apoelstra merged commit ba04d92 into rust-bitcoin:0.29.x Sep 11, 2024
18 of 21 checks passed
@apoelstra
Copy link
Member Author

Tagged and published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants